Skip to content

Add prefix to PSR-4 autoloader #108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 13, 2018

Conversation

dcloues
Copy link
Contributor

@dcloues dcloues commented May 25, 2018

PSR-4 autoloaders with no prefix can hurt performance, especially in large projects with many dependencies. When resolving class names to files, if Composer cannot use a prefix to limit the search paths, it has to search every autoloader with a blank prefix.

This change adds a prefix to the autoloader, to avoid this problem.

Running composer diagnose previously reported this:

~/src/php-sdk % composer diagnose
Checking composer.json: WARNING
Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance

With this change, composer diagnose is happy.

I couldn't add tests for this, because it's not a code change. But, I did run the test suite to verify that it everything still passes as expected.

PSR-4 autoloaders with no prefix can hurt performance, especially in
large projects with many dependencies. When resolving class names to
files, if Composer cannot use a prefix to limit the search paths, it has
to search every autoloader with a blank prefix.

This change adds a prefix to the autoloader, to avoid this problem.

Running `composer diagnose` previously reported this:

~/src/php-sdk % composer diagnose
Checking composer.json: WARNING
Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance

With this change, `composer diagnose` is happy.

I couldn't add tests for this, because it's not a code change. But, I
did run the test suite to verify that it everything still passes as
expected.
@optibot
Copy link

optibot commented May 25, 2018

Can one of the admins verify this patch?

1 similar comment
@optibot
Copy link

optibot commented May 25, 2018

Can one of the admins verify this patch?

@coveralls
Copy link

coveralls commented Jun 28, 2018

Coverage Status

Coverage decreased (-0.1%) to 96.942% when pulling ca1f77c on dcloues:composer-autoload-perf into b8ee905 on optimizely:master.

@aliabbasrizvi
Copy link
Contributor

Hi @dcloues , thank you for submitting this fix. I'd like to request you to fill out our CLA mentioned here: https://github.com/optimizely/php-sdk/blob/master/CONTRIBUTING.md#contributing-to-the-optimizely-php-sdk

Once done, I shall be able to get this in.

Thank you again.

@dcloues
Copy link
Contributor Author

dcloues commented Jun 29, 2018

Hi @aliabbasrizvi - thanks so much! Sorry for missing the CLA the first time around - I read the contributing guide and somehow read right past it, even though it's super clear. I just submitted the agreement - thanks again.

@aliabbasrizvi
Copy link
Contributor

build

1 similar comment
@aliabbasrizvi
Copy link
Contributor

build

@aliabbasrizvi
Copy link
Contributor

Thanks @dcloues for sending this in. Changes look good. I am going to merge this in and it will hopefully be available in the next release 🙂.

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aliabbasrizvi aliabbasrizvi merged commit 3280346 into optimizely:master Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants